fix(server): forward extension-mutated headers — they never reached the wire - #274
Conversation
…he wire
preForward builds reqCtx.headers = { ...clientReq.headers } for the
extension pipeline to read and mutate, but only reqCtx.body was ever
serialized back into the outbound request: forwardRequest still read
the ORIGINAL clientReq.headers, so added, changed and deleted header
keys were silently discarded. auto-1m-guard's strip mode is the
standing in-tree victim — its unit test asserts ctx.headers is mutated
correctly, and nothing proved the mutation reached the wire (it did
not).
preForward now returns the mutated header object, and handleMessages/
handleBootstrap forward a minimal { url, method, headers } wrapper —
forwardRequest only reads those three fields, so upstream.mjs's
signature is untouched. handlePassthrough runs no extension pipeline
and is deliberately unchanged. Returning the object itself (not a
copy) keeps deletions visible: plain object semantics carry add,
change and delete alike.
Wire-level regression test: a real proxy instance through the real
pipeline to a local upstream that records what it received — a
synthetic extension exercising add/change/delete, plus auto-1m-guard's
strip contract end-to-end. Against the unfixed server the suite fails
4 of 5; with the fix 5 of 5 pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![vsits-codex-review-agent[bot]](https://avatars.githubusercontent.com/u/115404?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: PR #274 header mutation forwarding
Date: 2026-07-31
Reviewed: proxy/server.mjs, proxy/upstream.mjs, test/proxy-server-header-propagation.test.mjs at 4f2eb482083c2f722e28ad2b587612bc70ca6fe7
Round: 1
Label applied: approved-by-codex-agent
What Is Correct
forwardRequestonly readsclientReq.url,clientReq.headers, andclientReq.methodfrom its first argument, so the new{ url, method, headers }wrapper is sufficient and does not break the upstream transport contract.- Abort propagation and streaming behavior remain intact because the abort signal is still passed separately to
forwardRequest, and streaming still operates on the returnedupstreamRes; the wrapped request object is not part of that path. content-lengthrecomputation still works:forwardRequestrebuilds upstream headers, then overwritescontent-lengthfrom the serializedbody, so extension-mutatedcontent-lengthvalues do not leak through.- Leaving
handlePassthroughunchanged is correct. That path intentionally does not run the extension pipeline, so there is no mutated header state to preserve there. - The new regression test is a real wire-path test, not an intermediate-state assertion. It starts a real proxy, runs the real extension pipeline, forwards to a local HTTP upstream, and asserts on the headers the upstream actually received.
- The author's “fails 4/5 on unfixed main” claim is accurate. Running
test/proxy-server-header-propagation.test.mjsagainst the merge-base (76d586d48255aa5eb5b3ff5aa7ced5baeff66640) produced exactly 1 pass / 4 failures; on the PR head it produced 5 passes / 0 failures. - Full suite result on the PR head was clean:
1435tests passed,0failed.
Blockers
None.
What Needs Attention
- Coverage is strongest on
/v1/messages; the identical bootstrap wrapper path is changed but not directly exercised by a new bootstrap-specific regression. I do not consider that a blocker because the wrapper contract was independently verified againstproxy/upstream.mjs, but it remains the only changed route without a direct test in this PR.
Bloat / Non-Functional
None.
Recommendations
- A follow-up bootstrap-focused regression test would close the remaining route-coverage gap by proving header mutations also reach the wire on
/api/claude_cli/bootstrap. - Keep relying on
buildUpstreamHeaders()as the sanitization boundary. This change intentionally allows trusted extensions to mutate end-to-end headers such asauthorization, buthost, hop-by-hop headers,proxy-*, and effectivecontent-lengthare still normalized there.
Bottom Line
The fix is correct and complete for the defect claimed in this PR. The request wrapper matches the actual forwardRequest contract, it preserves abort and streaming behavior, it keeps content-length authoritative, and the new regression test proves the bug on the old code and the fix on the new code at the wire level. This is safe to approve. — Codex review
vsits-codex-review-agent
Bot
added
reviewed-by-codex-agent
4f2eb48
0f8d82b to
4f2eb48
Compare
|
@Gunther-Schulz — apologies: our review agent pushed a commit to your branch, and it shouldn't have. While reviewing this PR it wrote its review artifact to I've force-pushed the branch back to Side effect worth flagging: the force-push dismissed the approving review (this repo dismisses stale reviews on push). The review's findings still stand — the dropped commit was documentation only and didn't touch code — so this needs a re-submission at the current head, not a re-review. That's on us to sort out, not you. On the substance: the finding is confirmed. Thanks for the catch, and for the wire-level test — asserting on what the upstream actually received is what made it undeniable. — Proxy Builder |
There was a problem hiding this comment.
Codex review: re-submitting approval after the fork branch was reverted to remove my docs-only review artifact commit.
I previously reviewed and approved this PR at 0f8d82b2. I confirmed the current head 4f2eb482 differs only by removal of docs/code-reviews/pr-274-round-1-codex.md; there is no code change.
vsits-codex-review-agent
Bot
added
reviewed-by-codex-agent
server.close() waits for in-flight requests, and a live session always has one (the streaming /v1/messages response) — so the 5s watchdog was the normal exit under a supervisor, not the exception. Exiting 1 there made every systemctl stop log status=1/FAILURE and tripped Restart=on-failure on a deliberate stop. The watchdog now force-closes lingering connections (closeAllConnections, Node >=18.2; feature-detected, prior behavior on the 18.0/18.1 floor), reports the forcing on stderr, and exits 0. Verified locally merged onto main with #274: full suite 1437/0. Closes #277
…286) The artifact rule said "PR review -> on the PR branch" without qualifying who owns it, so reviewing a community PR meant pushing to the contributor's branch. That happened on #274 and #277 this week; both were reverted. maintainerCanModify makes the push possible; it is not permission. It also backfires mechanically: this repo dismisses stale reviews on push, so the review-doc commit dismisses the approval it documents and staleness-flags the approval label against the new head. Fork PRs now have no committed artifact — the formal review body is the artifact. Closes #286
![vsits-proxy-builder[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
The global reviewer bar is "larger than the directive's requirements justify." Community PRs have no directive, so the bar had nothing to anchor to: across eight open community PRs every review reported "Bloat: None", including one on 6,630 lines of new production code. AGENTS.md now anchors no-directive PRs to the defect being fixed, and requires the size numbers be stated in every review — a number is checkable, "None" is not. Calibrated against #274/#277/#261, all merged and all proportionate, so the reference is real work rather than a guess. Explicitly excludes test volume and why-comments from the finding, since both are high here by design. CONTRIBUTING.md is new: AGENTS.md and CLAUDE.md were already committed but are addressed to our own agents, so contributors had no file that spoke to them. Points them at both, and asks PRs over ~300 production LOC to carry the non-functional checklist. Claude-Session: https://claude.ai/code/session_01GvZKP1JeXgHFCovTaAPT5B Co-authored-by: vsits-proxy-builder[bot] <223447982+vsits-proxy-builder[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
preForwardbuildsreqCtx.headers = { ...clientReq.headers }for extensions to read and mutate — but onlyreqCtx.bodyis serialized back into the outbound request.forwardRequeststill reads the originalclientReq.headers, so header mutations (added, changed, or deleted keys) are silently discarded.The in-tree victim is auto-1m-guard's strip mode: its unit test correctly asserts
ctx.headersis mutated, but nothing proved the mutation reached the wire — it didn't. Any extension that adds a beta token or strips a header is affected the same way.Fix:
preForwardreturns the mutated header object;handleMessages/handleBootstrapforward a minimal{ url, method, headers }wrapper (those are the only fieldsforwardRequestreads, soupstream.mjsis untouched).handlePassthroughruns no pipeline and is deliberately unchanged.Evidence: the new wire-level test spawns a real proxy through the real pipeline against a local upstream that records what it received — a synthetic add/change/delete extension plus auto-1m-guard's strip contract end-to-end. Against unfixed
mainit fails 4 of 5; with this patch 5 of 5 pass.Independent of #272/#273 (though #273's beta-header injection is what originally surfaced it).
🤖 Generated with Claude Code